test: Add serde tests for DTypes
#3205
Conversation
Will reuse them in `serde_test`
Very impressed this worked
|
I'm pleasantly suprised this all worked without changing a single line of (non-test) code 😅 I've even tried protocol 3 locally and still no issues. Can someone burst my bubble and break this? 🙏 |
There was a problem hiding this comment.
Hey @dangotbanned - thanks for the initiative! This looks 🔥 (no changes in the codebase at all, I am also surprised)
I left a comment that might simplify some tests.
And as:
- I know close to nothing about custom pickling
- My brain is cooked for the day
I am afraid that the following invite will need to wait for another day or someone else
Can someone burst my bubble and break this? 🙏
😂
| class Identity(Protocol): | ||
| def __call__(self, obj: IntoDTypeT, /) -> IntoDTypeT: ... | ||
|
|
||
|
|
||
| def _roundtrip_pickle(protocol: int | None = None) -> Identity: | ||
| def fn(obj: IntoDTypeT, /) -> IntoDTypeT: | ||
| result: IntoDTypeT = pickle.loads(pickle.dumps(obj, protocol)) | ||
| return result | ||
|
|
||
| return fn | ||
|
|
||
|
|
||
| @pytest.fixture( | ||
| params=[_roundtrip_pickle(), _roundtrip_pickle(4), _roundtrip_pickle(5)], | ||
| ids=["pickle-None", "pickle-4", "pickle-5"], | ||
| ) | ||
| def roundtrip(request: pytest.FixtureRequest) -> Identity: | ||
| fn: Identity = request.param | ||
| return fn |
There was a problem hiding this comment.
The idea I had here is we can pretty easily extend this by adding more functions that can roundtrip.
pickle is all that I've used, but the protocols which are being called can be used elsewhere
import narwhals as nw
dtype = nw.Struct({"a": nw.List(nw.Array(nw.String, 5))})
>>> dtype.__reduce_ex__(5)
(<function copyreg.__newobj__(cls, *args)>,
(narwhals.dtypes.Struct,),
(None,
{'fields': [Field('a', List(Array(<class 'narwhals.dtypes.String'>, shape=(5,))))]}),
None,
None)
>>> dtype.__getstate__()
(None,
{'fields': [Field('a', List(Array(<class 'narwhals.dtypes.String'>, shape=(5,))))]})There was a problem hiding this comment.
Barely on topic, but noticable from the example I used
@FBruzzesi nested DTypes are where the metaclass repr from polars would be nice
import narwhals as nw
>>> nw.Struct({"a": nw.List(nw.Array(nw.String, 5))})
Struct({'a': List(Array(<class 'narwhals.dtypes.String'>, shape=(5,)))})import polars as pl
>>> pl.Struct({"a": pl.List(pl.Array(pl.String, 5))})
Struct({'a': List(Array(String, shape=(5,)))})
Co-authored-by: Francesco Bruzzesi <[email protected]>
…into dtype-serde
There was a problem hiding this comment.
@dangotbanned if you are happy with the state of this, I think we can merge these changes for now and iterate on top of them if needed.
Quoting my previous self #3205 (review)
I know close to nothing about custom pickling
Thanks @FBruzzesi!
That shouldn't be an issue, since we're utilizing the default picking
|
What type of PR is this? (check all applicable)
Related issues
__slots__to allDTypes #3194Checklist
If you have comments or can explain your changes, please do so below
Starting off fairly small here, trying to work out if we'll need to implement any other dunders mentioned in Pickling Class Instances
I
havehad a feelingEnumwith deferred categories might need some work.But, turns out it worked without changes 🥳 (b2ba749)